Skip to content

fix: refactor clear icon to button element to make it accessible#1224

Merged
afc163 merged 3 commits into
react-component:masterfrom
Pareder:fix/clear-btn-accessibility
Jun 26, 2026
Merged

fix: refactor clear icon to button element to make it accessible#1224
afc163 merged 3 commits into
react-component:masterfrom
Pareder:fix/clear-btn-accessibility

Conversation

@Pareder

@Pareder Pareder commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This PR improves clear button functionality by:

  • changing custom div to navite button element
  • using aria-label attribute by extending allowClear object type

Summary by CodeRabbit

发行说明

  • 新功能
    • allowClear 现在可同时提供自定义清除按钮标签(用于无障碍展示)。
  • 改进
    • 清除按钮已改为原生 button,支持键盘触发;自动补全并传递清除按钮的无障碍 aria-label
    • 交互优化:清除点击不再打开下拉,并更好地维持输入焦点行为。
  • 测试
    • 更新并补充清除按钮的鼠标/键盘行为与回调触发用例。

@vercel

vercel Bot commented Jun 8, 2026

Copy link
Copy Markdown

@Pareder is attempting to deploy a commit to the React Component Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 969d9dcb-ed49-47fb-92d6-bddca954e00e

📥 Commits

Reviewing files that changed from the base of the PR and between c12b0fe and 7aa689a.

⛔ Files ignored due to path filters (1)
  • tests/__snapshots__/Select.test.tsx.snap is excluded by !**/*.snap
📒 Files selected for processing (3)
  • src/SelectInput/index.tsx
  • src/hooks/useAllowClear.tsx
  • tests/Select.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/hooks/useAllowClear.tsx
  • src/SelectInput/index.tsx
  • tests/Select.test.tsx

概览

PR 为 Select 组件的清除按钮增加 label/clearLabel 数据流,改用原生 button 渲染清除控件,并将清除触发调整为 click 处理。

变更内容

Clear 按钮可访问性标签支持

层 / 文件(s) 概述
类型和配置扩展
src/BaseSelect/index.tsx, src/hooks/useAllowClear.tsx, src/SelectInput/index.tsx
allowClear 配置、AllowClearConfigSelectInputProps 新增 label / clearLabel 相关字段,清除标签属性进入组件契约。
useAllowClear 与 BaseSelect 传递
src/hooks/useAllowClear.tsx, src/BaseSelect/index.tsx
useAllowClear 返回清除标签,BaseSelect 解构该值并传递给 SelectInput
SelectInput 清除按钮重构
src/SelectInput/index.tsx
清除区域改为原生 button,使用 aria-label,并将清除回调切换为 click 触发,同时保留原有清除图标渲染。
测试覆盖
tests/Select.test.tsx, tests/shared/allowClearTest.tsx
更新焦点与清除触发测试,新增 button/aria-label、mousedown 默认行为、下拉不打开和键盘激活清除等断言。

可能相关的 PR

建议的审查人

  • afc163
  • zombieJ

🐰 兔子轻跳过草坡,
清除按钮戴上名牌啰,
click 一响,值归零,
键盘一按也通畅多,
绿色小耳朵听得清,
交互顺了心也活泼。

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题准确概括了将清除图标重构为 button 以提升可访问性的主要改动。
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

src/SelectInput/index.tsx

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.

src/hooks/useAllowClear.tsx

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.

tests/Select.test.tsx

ESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.44%. Comparing base (c12c390) to head (7aa689a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1224   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files          31       31           
  Lines        1270     1271    +1     
  Branches      465      466    +1     
=======================================
+ Hits         1263     1264    +1     
  Misses          7        7           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves the accessibility and keyboard navigation of the clear button in the Select component by changing its markup from a div to a native button element and adding support for an accessible label. Feedback on these changes suggests adding a default fallback for the label to prevent type contract violations and ensure accessibility. Additionally, minor cleanups are recommended in the tests to remove an unused mock and simplify click event dispatching.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/hooks/useAllowClear.tsx Outdated
return {
allowClear: mergedAllowClear,
clearIcon: mergedAllowClear ? allowClearConfig.clearIcon || clearIcon || '×' : null,
label: mergedAllowClear ? allowClearConfig.label : '',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The AllowClearConfig interface defines label as a required string. However, when allowClear is passed as true or as an object without a label property, allowClearConfig.label will be undefined. Returning allowClearConfig.label directly violates the type contract and leaves the clear button without an accessible name for screen readers.\n\nProviding a default fallback like 'clear' resolves the type mismatch and ensures the button remains accessible by default.

Suggested change
label: mergedAllowClear ? allowClearConfig.label : '',
label: mergedAllowClear ? allowClearConfig.label || 'clear' : '',

Comment thread tests/Select.test.tsx
Comment on lines +2511 to 2512
const clickPreventDefault = jest.fn();
const { container } = render(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The clickPreventDefault mock is unused because the component does not call preventDefault() on the click event (it only calls it on the mousedown event). Removing this definition avoids unused variable warnings and simplifies the test setup.

    const { container } = render(

Comment thread tests/Select.test.tsx
Comment on lines +2521 to +2523
const clickEvent = createEvent.click(container.querySelector('.rc-select-clear'));
clickEvent.preventDefault = clickPreventDefault;
fireEvent(container.querySelector('.rc-select-clear'), clickEvent);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since we don't need to mock preventDefault on the click event, we can simplify the event dispatching by directly calling fireEvent.click.

Suggested change
const clickEvent = createEvent.click(container.querySelector('.rc-select-clear'));
clickEvent.preventDefault = clickPreventDefault;
fireEvent(container.querySelector('.rc-select-clear'), clickEvent);
fireEvent.click(container.querySelector('.rc-select-clear'));

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/hooks/useAllowClear.tsx`:
- Line 41: The clear button label can be undefined when callers pass allowClear
or clearIcon without allowClear.label; update useAllowClear to provide a
non-empty fallback string for the button label (e.g. a default like "Clear" or
an i18n key) instead of passing allowClearConfig.label directly—locate the
mergedAllowClear construction and the place where label: mergedAllowClear ?
allowClearConfig.label : '' is set and replace it so label is always a safe,
non-empty string (refer to mergedAllowClear and allowClearConfig.label in
useAllowClear).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0e63c184-4f12-49e8-b5e1-a7e46891d276

📥 Commits

Reviewing files that changed from the base of the PR and between 7876c6c and c12b0fe.

⛔ Files ignored due to path filters (1)
  • tests/__snapshots__/Select.test.tsx.snap is excluded by !**/*.snap
📒 Files selected for processing (5)
  • src/BaseSelect/index.tsx
  • src/SelectInput/index.tsx
  • src/hooks/useAllowClear.tsx
  • tests/Select.test.tsx
  • tests/shared/allowClearTest.tsx

Comment thread src/hooks/useAllowClear.tsx Outdated
@Pareder

Pareder commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@afc163 @zombieJ @meet-student @QDyanbing Could you please review?

1 similar comment
@Pareder

Pareder commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@afc163 @zombieJ @meet-student @QDyanbing Could you please review?

@afc163

afc163 commented Jun 26, 2026

Copy link
Copy Markdown
Member

Thanks for working on this. This is the right lower-level change for ant-design/ant-design#58267: Ant Design can pass the localized clear text through allowClear.label, while rc-select owns the native button behavior.

One thing still needs to be fixed here before this is ready: useAllowClear should provide a non-empty fallback label when consumers use allowClear as true or { clearIcon }. Today AllowClearConfig.label is typed as string, but allowClearConfig.label can be undefined, so rc-select standalone can still render a clear button without an accessible name.

Could you update it to something like:

label: mergedAllowClear ? allowClearConfig.label ?? 'Clear' : '',

The existing button/click behavior and tests look aligned with what Ant Design needs. After this lands and is released, we can bump @rc-component/select in ant-design#58267 and wire the localized label there.

@Pareder

Pareder commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@afc163 Done.

@afc163 afc163 merged commit a0bd83e into react-component:master Jun 26, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants